expand info/debug/warning msgs if USB device `serial_number` not readable #423

jllanfranchi · 2024-04-07T14:23:58Z

if INSTR USB device,

say that device is USB INSTR that failed in warning message
catch exception
debug message with traceback if logging level is DEBUG
info message 1-line error summary if logging level is INFO
if path f"/dev/bus/usb/{dev.bus:03d}/{dev.address:03d}" exists, check permissions on that file & refer to FAQ link (known issue on Linux)

if RAW USB device,

say that device is USB RAW that failed in warning message

I will work on documentation fixes now, but wanted to get this in front of you for feedback in the meantime, @arr-ee (et al.).

Working to close pyvisa issue #758
Executed black . && isort -c . && flake8 with no errors black & isort OK; flake8 doesn't like a few lines' lengths
The change is fully covered by automated unit tests (TODO; feedback for what this could be?)
Documented in docs/ as appropriate (TODO)
Added an entry to the CHANGES file (TODO)

…readable if INSTR USB device, - say that device is USB INSTR that failed in warning message - catch exception - debug message with traceback if logging level is DEBUG - info message 1-line error summary if logging level is INFO - if path f"/dev/bus/usb/{dev.bus:03d}/{dev.address:03d}" exists, check permissions on that file & refer to FAQ link (known issue on Linux) if RAW USB device, - say that device is USB RAW that failed in warning message

jllanfranchi · 2024-04-08T11:00:53Z

Examples of messages running the code

import logging
import pyvisa

logging.basicConfig(level=logging.DEBUG)
rm = pyvisa.ResourceManager("@py")
rm.list_resources()

on Linux with permissions /dev/usb/001/016 not set correctly (I also have a USB RAW device on my system, so you can see the modified message for that as well).

logging level set to WARN:

WARNING:pyvisa:Found a USB INSTR device whose serial number cannot be read. The partial VISA resource name is: USB0::4883::32888::???::0::INSTR
WARNING:pyvisa:User does not have permission to write to /dev/bus/usb/001/016, so the above USB INSTR device cannot be used by pyvisa; see https://pyvisa.readthedocs.io/projects/pyvisa-py/en/latest/faq.html for more info.
WARNING:pyvisa:Found a USB RAW device whose serial number cannot be read. The partial VISA resource name is: USB0::1027::24593::???::0::RAW

logging level set to INFO:

WARNING:pyvisa:Found a USB INSTR device whose serial number cannot be read. The partial VISA resource name is: USB0::4883::32888::???::0::INSTR
INFO:pyvisa:Error raised from underlying module (pyusb): ValueError: The device has no langid (permission issue, no string descriptors supported or device error)
WARNING:pyvisa:User does not have permission to write to /dev/bus/usb/001/016, so the above USB INSTR device cannot be used by pyvisa; see https://pyvisa.readthedocs.io/projects/pyvisa-py/en/latest/faq.html for more info.
WARNING:pyvisa:Found a USB RAW device whose serial number cannot be read. The partial VISA resource name is: USB0::1027::24593::???::0::RAW

with logging level set to DEBUG:

DEBUG:pyvisa:SerialSession was correctly imported.
DEBUG:pyvisa:USBSession and USBRawSession were correctly imported.
DEBUG:pyvisa:TCPIPSession was correctly imported.
DEBUG:pyvisa:GPIBSession was not imported No module named 'gpib'.
DEBUG:pyvisa:Created library wrapper for py
DEBUG:pyvisa:Created ResourceManager with session 1292698
WARNING:pyvisa:Found a USB INSTR device whose serial number cannot be read. The partial VISA resource name is: USB0::4883::32888::???::0::INSTR
DEBUG:pyvisa:Traceback:
DEBUG:pyvisa:----------
DEBUG:pyvisa:Traceback (most recent call last):
DEBUG:pyvisa:
DEBUG:pyvisa:  File "/home/justinl/src/pyvisa-py/pyvisa_py/usb.py", line 282, in list_resources
DEBUG:pyvisa:    serial = dev.serial_number
DEBUG:pyvisa:
DEBUG:pyvisa:  File "/home/justinl/miniconda3/envs/py310/lib/python3.10/site-packages/usb/core.py", line 864, in serial_number
DEBUG:pyvisa:    self._serial_number = util.get_string(self, self.iSerialNumber)
DEBUG:pyvisa:
DEBUG:pyvisa:  File "/home/justinl/miniconda3/envs/py310/lib/python3.10/site-packages/usb/util.py", line 313, in get_string
DEBUG:pyvisa:    raise ValueError("The device has no langid"
DEBUG:pyvisa:
DEBUG:pyvisa:ValueError: The device has no langid (permission issue, no string descriptors supported or device error)
DEBUG:pyvisa:
DEBUG:pyvisa:----------
WARNING:pyvisa:User does not have permission to write to /dev/bus/usb/001/016, so the above USB INSTR device cannot be used by pyvisa; see https://pyvisa.readthedocs.io/projects/pyvisa-py/en/latest/faq.html for more info.
WARNING:pyvisa:Found a USB RAW device whose serial number cannot be read. The partial VISA resource name is: USB0::1027::24593::???::0::RAW
DEBUG:asyncio:Using selector: EpollSelector

arr-ee

Thank you for tackling this! Left some notes, will test the code in a bit.

Please note I do not have authority over the code, @MatthieuDartiailh has the last word. My comments are suggestions at best.

arr-ee · 2024-04-08T13:41:12Z

pyvisa_py/usb.py

@@ -292,6 +295,39 @@ def list_resources() -> List[str]:
 "usb_interface_number": intfc,
 },
 )
+ logging_level = logger.getEffectiveLevel()
+ if logging_level <= logging.DEBUG:
+ if exc_strs := traceback.format_exception(err):


what's the benefit of doing this over logger.debug("error while reading serial number", exc_info=err)?

I am also not sure if this amount of pre-processing is worth it. On DEBUG I'd prefer to have all exceptions logged in general. For higher levels, we can just add the exception message to the warning's dict maybe?

I was unaware of that kwarg to logger methods! Will change to using exc_info.

I agree with @arr-ee here. Please update the code accordingly.

arr-ee · 2024-04-08T13:53:46Z

pyvisa_py/usb.py

+ exc_strs[0].strip(),
+ )
+
+ dev_path = f"/dev/bus/usb/{dev.bus:03d}/{dev.address:03d}"


this is linux-specific and should be gated as such (see https://github.com/pyvisa/pyvisa/blob/main/pyvisa/util.py#L1005 for example).

maybe also move the check logic into a helper method in usbutil?

As to your second point, particularly if we are going to implement this for both USB INSTR and USB RAW devices, then it certainly makes sense to move the check logic into a separate module. But I can do that anyway even just to keep the code here cleaner, omitting the gory details that are only pertinent to one OS.

As to your first comment, I thought about gating based on platform. (I also tried to figure out if there's an equivalent for FreeBSD/OS X, but failed to find that.) But I figured by the time I went through all the logic required to check that the sys.platform was something this check could work on, I could do it the Python-ic (duck-typing) way and simply look for the file's existence, check the file's permissions, and go from there, and that would essentially take care of "does this check work on my particular platform?" issues.

I'm certainly not married to that way, I get why one might not want to use the duck-type perspective for this check. This almost certainly only works on Linux & nowhere else, and maybe we discover a Linux where it doesn't work, and then we would just want to add to the gating condition. So, yeah, I'll put the check within a if sys.platform.startswith("linux"): ... check.

But I can do that anyway even just to keep the code here cleaner, omitting the gory details that are only pertinent to one OS.

Yup, plus it will be easier to test (that's a tricky thing though).

But I figured by the time I went through all the logic required to check that the sys.platform was something this check could work on, I could do it the Python-ic (duck-typing) way and simply look for the file's existence, check the file's permissions, and go from there, and that would essentially take care of "does this check work on my particular platform?" issues.

This is fair! I prefer an explicit check to make the intent explicit to the readers: "this is only for linux", but my disregard for pythonic way of doing things can and should be questioned :)

I also tried to figure out if there's an equivalent for FreeBSD/OS X, but failed to find that.

Yeah it's very system-specific, and I don't think we'd have permissions issue on macos (we'd have other ones!). I was wondering if there's a "better" way to get to the device file on linux as well, but it seems that string magic should do the trick until proven otherwise.

Here also I think an explicit check will make the intent clearer for people looking at it.

arr-ee · 2024-04-08T13:54:22Z

pyvisa_py/usb.py

@@ -336,7 +372,7 @@ def list_resources() -> List[str]:
 serial = dev.serial_number
 except (NotImplementedError, ValueError, usb.USBError):
 msg = (
- "Found a device whose serial number cannot be read."
+ "Found a USB RAW device whose serial number cannot be read."


can't we run into the same permissions issue here?

We do. I thought pyvisa didn't care about RAW devices (because it doesn't control them), but I realize now that assumption may be incorrect.

USB RAW are weird because there are supported by NI but are not part of the IVI specs. If we can provide more information we should though.

I observed in pyvisa issue Linux host and getting permissions to talk to a USBTMC device
USBTMC devices show up as both /dev/usbtmcN and /dev/bus/usb/NNN/NNN
Both instances need to have suitable permissions for pyvisa with pyvisa-py to be able to communicate

If only the /dev/usbtmcN has permissions then you get WARNING Found a device whose serial number cannot be read

arr-ee · 2024-04-08T13:55:19Z

pyvisa_py/usb.py

+ logger.warning(
+ "User does not have permission to %s %s, so the above USB INSTR"
+ " device cannot be used by pyvisa; see"
+ " https://pyvisa.readthedocs.io/projects/pyvisa-py/en/latest/faq.html"


should link to the specific anchor once docs are updated

Only issue with that is we need to keep the two in sync; a change in the docs will necessitate a change in the code. That could be annoying for a maintainer, so I left it out. But if that's what you want, I'm happy add the anchor to the link.

I doubt that docs organisation will change drastically enough to make this a big issue, and since we're going to the trouble of linking we might as well link to the exact spot.

I agree a direct link would be better. Anything that can reduce the number of obscure bug reports is something I am happy to include.

jllanfranchi · 2024-04-08T17:01:39Z

Really appreciate all your helpful feedback, @arr-ee . I will push updates according to your comments ASAP, and hopefully have a first pass at adding to the docs that can be reviewed as well.

I want to re-iterate how helpful the feedback you gave is, not really being aware of all the ins-and-outs of pyvisa & pyvisa-py (like code organization in the usbutils module; wouldn't have thought of that as an outsider).

MatthieuDartiailh

Looks good. Once the minor requested changes are in I will merge.

MatthieuDartiailh · 2024-04-15T16:07:22Z

pyvisa_py/usb.py

@@ -292,6 +295,39 @@ def list_resources() -> List[str]:
 "usb_interface_number": intfc,
 },
 )
+ logging_level = logger.getEffectiveLevel()
+ if logging_level <= logging.DEBUG:
+ if exc_strs := traceback.format_exception(err):


I agree with @arr-ee here. Please update the code accordingly.

MatthieuDartiailh · 2024-04-15T16:08:58Z

pyvisa_py/usb.py

+ exc_strs[0].strip(),
+ )
+
+ dev_path = f"/dev/bus/usb/{dev.bus:03d}/{dev.address:03d}"


Here also I think an explicit check will make the intent clearer for people looking at it.

MatthieuDartiailh · 2024-04-15T16:10:23Z

pyvisa_py/usb.py

+ logger.warning(
+ "User does not have permission to %s %s, so the above USB INSTR"
+ " device cannot be used by pyvisa; see"
+ " https://pyvisa.readthedocs.io/projects/pyvisa-py/en/latest/faq.html"


I agree a direct link would be better. Anything that can reduce the number of obscure bug reports is something I am happy to include.

MatthieuDartiailh · 2024-04-15T16:11:09Z

pyvisa_py/usb.py

@@ -336,7 +372,7 @@ def list_resources() -> List[str]:
 serial = dev.serial_number
 except (NotImplementedError, ValueError, usb.USBError):
 msg = (
- "Found a device whose serial number cannot be read."
+ "Found a USB RAW device whose serial number cannot be read."


USB RAW are weird because there are supported by NI but are not part of the IVI specs. If we can provide more information we should though.

MatthieuDartiailh · 2024-05-27T15:47:08Z

@jllanfranchi any idea when you will be able to address the comments ?

MatthieuDartiailh · 2024-10-01T16:38:20Z

Friendly ping @jllanfranchi

jllanfranchi added 2 commits April 7, 2024 10:05

Change message from USBTMC -> USB INSTR for clarity

f7a08a7

jllanfranchi mentioned this pull request Apr 8, 2024

add -v cmdline flag to pyvisa-shell for warn/info/debug logging output pyvisa/pyvisa#816

Merged

5 tasks

arr-ee suggested changes Apr 8, 2024

View reviewed changes

MatthieuDartiailh requested changes Apr 15, 2024

View reviewed changes

arr-ee mentioned this pull request Sep 30, 2024

Fails to find connected devices pyvisa/pyvisa#858

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expand info/debug/warning msgs if USB device `serial_number` not readable #423

expand info/debug/warning msgs if USB device `serial_number` not readable #423

jllanfranchi commented Apr 7, 2024

jllanfranchi commented Apr 8, 2024

arr-ee left a comment •

edited

Loading

arr-ee Apr 8, 2024

jllanfranchi Apr 8, 2024

MatthieuDartiailh Apr 15, 2024

arr-ee Apr 8, 2024

jllanfranchi Apr 8, 2024

arr-ee Apr 8, 2024

MatthieuDartiailh Apr 15, 2024

arr-ee Apr 8, 2024

jllanfranchi Apr 8, 2024

MatthieuDartiailh Apr 15, 2024

DavidLutton Oct 12, 2024

arr-ee Apr 8, 2024

jllanfranchi Apr 8, 2024

arr-ee Apr 8, 2024

MatthieuDartiailh Apr 15, 2024

jllanfranchi commented Apr 8, 2024

MatthieuDartiailh left a comment

MatthieuDartiailh Apr 15, 2024

MatthieuDartiailh Apr 15, 2024

MatthieuDartiailh Apr 15, 2024

MatthieuDartiailh Apr 15, 2024

MatthieuDartiailh commented May 27, 2024

MatthieuDartiailh commented Oct 1, 2024

expand info/debug/warning msgs if USB device serial_number not readable #423

Are you sure you want to change the base?

expand info/debug/warning msgs if USB device serial_number not readable #423

Conversation

jllanfranchi commented Apr 7, 2024

jllanfranchi commented Apr 8, 2024

arr-ee left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jllanfranchi commented Apr 8, 2024

MatthieuDartiailh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthieuDartiailh commented May 27, 2024

MatthieuDartiailh commented Oct 1, 2024

expand info/debug/warning msgs if USB device `serial_number` not readable #423

expand info/debug/warning msgs if USB device `serial_number` not readable #423

arr-ee left a comment •

edited

Loading